Introduce RouteParametersConfig #3342

shaavan · 2024-09-26T12:33:19Z

Partially addresses #3262

Introduce a struct that allows the user to configure the parameters for routing the payment.
Use the struct for BOLT12 payment flow.

shaavan · 2024-09-26T12:38:02Z

A gentle ping! Would love to get your feedback or approach ACK before moving forward with testing and applying a similar approach on the BOLT11 side.

Thanks a lot!

codecov · 2024-09-26T12:40:07Z

Codecov Report

Attention: Patch coverage is 82.14286% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.61%. Comparing base (ad19d93) to head (a608b55).

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	82.14%	3 Missing and 2 partials ⚠️
lightning/src/routing/router.rs	85.00%	0 Missing and 3 partials ⚠️
lightning/src/ln/channelmanager.rs	75.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3342      +/-   ##
==========================================
- Coverage   89.61%   89.61%   -0.01%     
==========================================
  Files         127      127              
  Lines      103533   103569      +36     
  Branches   103533   103569      +36     
==========================================
+ Hits        92785    92811      +26     
- Misses       8051     8053       +2     
- Partials     2697     2705       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vincenzopalazzo

LGTM overall.

However I am not the right person to judge if the PR is what the ldk team is looking for.

For instance the following is confusing me :) but I think this is a discussion to be done inside the issue

They also don't really make sense in a BOLT 11 world if we pass a Bolt11Invoice directly to ChannelManager (which we should do now that lightning depends on lightning-invoice).

BTW Nice git history!

P.S: I left some API idea comment in some commit review, idk if they are useful or not. probably they will remove a lot of map(_ {}) code, but I had to write the code to confirm

lightning/src/routing/router.rs

lightning/src/ln/outbound_payment.rs

vincenzopalazzo · 2024-10-02T16:13:15Z

lightning/src/ln/channelmanager.rs

@@ -9303,7 +9309,7 @@ where
 	pub fn pay_for_offer(
 		&self, offer: &Offer, quantity: Option<u64>, amount_msats: Option<u64>,
 		payer_note: Option<String>, payment_id: PaymentId, retry_strategy: Retry,
-		max_total_routing_fee_msat: Option<u64>
+		manual_routing_params: Option<ManualRoutingParameters>
 	) -> Result<(), Bolt12SemanticError> {


OK no needed option here but: what do you think about having the function call like pay_for_offer(offer, OfferParams::default()) where OfferParams is something like that

struct OfferParams { quantity: Option<u64>, amount_msats: Option<u64>, manual_routing_params: Option<ManualRoutingParameters>, }

I think is a lot of more verbose in this way, but I also this that this will be a lot of cleaner.

Now I do not think this will fit well in this PR maybe can be a followup one, but with this patter, it is possible to hide future updates to offers (e.g: recurrence or market place feature) without breaking too much the code

I think this is a great improvement! It will help keep the function signature cleaner while remaining maintainable for future parameter increases.

Maybe we can take this in a follow-up. @TheBlueMatt, what do you think?

No strong opinions.

lightning/src/ln/outbound_payment.rs

TheBlueMatt

High-level design looks right to me.

TheBlueMatt · 2024-10-08T18:16:09Z

lightning/src/ln/channelmanager.rs

@@ -9303,7 +9309,7 @@ where
 	pub fn pay_for_offer(
 		&self, offer: &Offer, quantity: Option<u64>, amount_msats: Option<u64>,
 		payer_note: Option<String>, payment_id: PaymentId, retry_strategy: Retry,
-		max_total_routing_fee_msat: Option<u64>
+		manual_routing_params: Option<ManualRoutingParameters>
 	) -> Result<(), Bolt12SemanticError> {


No strong opinions.

lightning/src/routing/router.rs

lightning/src/ln/outbound_payment.rs

shaavan · 2024-10-13T13:31:59Z

Updated from pr3342.01 to pr3342.02 (diff):

Changes:

Rebase on main

shaavan · 2024-10-13T15:30:12Z

Updated from pr3342.02 to pr3342.03 (diff):
Addressed @vincenzopalazzo, @jkczyz, @TheBlueMatt comments

Changes:

Renamed ManualRoutingParameters -> RouteParametersOverride
Reintroduce the max_total_routing_fee_msat in AwaitingInvoice to support downgrading.
Update API, so that the PaymentParameters are created using the Overrides, and then the RouteParameters are created using it.

lightning/src/routing/router.rs

lightning/src/ln/outbound_payment.rs

TheBlueMatt · 2024-10-16T23:48:21Z

lightning/src/ln/outbound_payment.rs

+		// Deprecated: Retained for backward compatibility.
+		// If set during read, this field overrides `RouteParameters::max_total_routing_fee_msat`
+		// instead of `RouteParametersOverride::max_total_routing_fee_msat`.
 		max_total_routing_fee_msat: Option<u64>,


Ugh, I really prefer to handle this kind of thing at the serialization layer rather than in our logic. It'll be pretty annoying to break out impl_writeable_tlv_based_enum_upgradable! here, though....I wonder if we shouldn't try to (yet again) expand the featureset of our enum-deser logic. Lets get this PR ready in every other way and then I can play with it, maybe in a followup.

Done! 🚀 Thanks a lot!

TheBlueMatt · 2024-10-16T23:51:25Z

Looks like most of my previous comments went unaddressed? Just checking cause I interpreted your comment as you addressing them.

shaavan · 2024-10-18T12:59:08Z

Updated from pr3342.03 to pr3342.04 (diff):
Addressed @TheBlueMatt’s comments:

Changes:

Update Role of RouteParamsOverride:

Instead of presenting this new struct as just overriding the PaymentParams, it is now structured as allowing the user to set configuration parameters.
This brings some changes to the struct:
- It has been renamed from RouteParamsOverride to RouteParamsConfig.
- The parameters are now required rather than optional.
- Documentation has been expanded to make the struct stand independently of {Route, Payment} Params.

Cleanups:

Introduced a new separate function, with_user_config, in PaymentParams, making the function more modular and focused.
RouteParamsConfig is now a required field in AwaitingInvoice and InvoiceReceived, initialized with default values if absent. This eliminates redundant optioning and switches to default values where necessary.

shaavan · 2024-10-18T13:02:31Z

@TheBlueMatt

Looks like most of my previous comments went unaddressed? Just checking cause I interpreted your comment as you addressing them.

Hey Matt, I’m really sorry for missing your previous comments – not sure how that slipped through! I've gone through everything now and made sure to address them all in the latest update. Thanks so much for your patience! 🌟

lightning/src/ln/outbound_payment.rs

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2024-10-22T00:05:56Z

Okay, I probably spent a bit too long on it, but #3378 should let us drop the legacy fields in outbound_payment.rs at de-serialization time.

shaavan · 2024-10-22T11:46:43Z

Updated from pr3342.04 to pr3342.05 (diff):

Changes:

Rebase on main. Rest no changes.

With the current architecture, `pay_for_offer` only allows setting `max_total_routing_fee_msat` as a route parameter. However, it doesn't provide users the flexibility to set other important parameters. This commit introduces a new struct, `RouteParametersConfig`, that optionally allows users to set additional routing parameters. In later commits, this struct will be utilized when paying BOLT12 invoices.

When `pay_for_offer` is called, it creates a new `PendingOutboundPayment` entry with relevant values that will be used when the corresponding invoice is received. This update modifies `AwaitingInvoice` to include the entire `RouteParametersConfig` struct instead of just `max_total_routing_fee_msat`. This change ensures all manual routing parameters are available when finding payment routes. Decisions & Reasoning: 1. **Retention of `max_total_routing_fee_msat` in `AwaitingInvoice` & `InvoiceReceived`** This field is retained to ensure downgrade support. 2. **Introduction of `route_params_config` in `InvoiceReceived`:** This was added for the same reason that `max_total_routing_fee_msat` was originally introduced in PR lightningdevkit#2417. The documentation has been updated to reflect this, based on [this comment](lightningdevkit@d7e2ff6#r1334619765).

This update allows users to call `pay_for_offer` with a set of parameters they wish to manually set for routing the corresponding invoice. By accepting `RouteParametersOverride`, users gain greater control over the routing process.

shaavan · 2024-10-22T12:08:16Z

Updated from pr3342.05 to pr3342.06 (diff):
Addressed @jkczyz, @TheBlueMatt comments

Changes:

Fix the comment regarding supporting standard behaviour during upgrade.
Set the max_total_routing_fee_msat along with route_params config to support downgrade.
Fix the fn static_invoice_received to ensure proper compilation and behaviour async_payment case as well.
Refactor the code to use if let instead of map for better readability.
Introduce the full config parameter for create_refund_builder as well.
Structured the commits slightly to have better logical progression.

TheBlueMatt · 2024-10-22T19:00:07Z

Lets get a review pass or two on #3378 and see if we want to move forward with it. If we do, we should rebase this on that and do the conversion in outbound_payment.rs at deserialization time.

vincenzopalazzo reviewed Oct 2, 2024

View reviewed changes

vincenzopalazzo mentioned this pull request Oct 2, 2024

BOLT12 sending doesn't allow to override {Route,Payment}Parameters #3262

Open

jkczyz self-requested a review October 3, 2024 16:03

jkczyz reviewed Oct 8, 2024

View reviewed changes

lightning/src/ln/outbound_payment.rs Outdated Show resolved Hide resolved

lightning/src/ln/outbound_payment.rs Show resolved Hide resolved

TheBlueMatt reviewed Oct 8, 2024

View reviewed changes

shaavan force-pushed the i3262 branch from 271522d to 0fc35e1 Compare October 13, 2024 13:31

shaavan force-pushed the i3262 branch from 0fc35e1 to a608b55 Compare October 13, 2024 15:30

shaavan changed the title ~~Introduce ManualRoutingParameters~~ Introduce RouteParametersOverride Oct 14, 2024

TheBlueMatt reviewed Oct 16, 2024

View reviewed changes

shaavan force-pushed the i3262 branch 2 times, most recently from 7cdb03f to 63b08a4 Compare October 18, 2024 12:58

shaavan changed the title ~~Introduce RouteParametersOverride~~ Introduce RouteParametersConfig Oct 18, 2024

jkczyz reviewed Oct 21, 2024

View reviewed changes

TheBlueMatt reviewed Oct 22, 2024

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

shaavan force-pushed the i3262 branch from 63b08a4 to d744f38 Compare October 22, 2024 11:46

shaavan added 6 commits October 22, 2024 17:17

f: Retain max_total_routing_fee_msat value

db71b52

Extend pay_for_offer to accept RouteParametersOverride

6ccb7b6

This update allows users to call `pay_for_offer` with a set of parameters they wish to manually set for routing the corresponding invoice. By accepting `RouteParametersOverride`, users gain greater control over the routing process.

Extend create_refund_builder to accept RouteParametersOverride

2e60588

Use RouteParametersConfig to create new RouteParameters

a5cd262

f: fix comment

4221929

shaavan force-pushed the i3262 branch from d744f38 to 4221929 Compare October 22, 2024 12:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce RouteParametersConfig #3342

Introduce RouteParametersConfig #3342

shaavan commented Sep 26, 2024 •

edited

Loading

shaavan commented Sep 26, 2024

codecov bot commented Sep 26, 2024 •

edited

Loading

vincenzopalazzo left a comment

vincenzopalazzo Oct 2, 2024

shaavan Oct 3, 2024

TheBlueMatt Oct 8, 2024

TheBlueMatt left a comment

TheBlueMatt Oct 8, 2024

shaavan commented Oct 13, 2024

shaavan commented Oct 13, 2024

TheBlueMatt Oct 16, 2024

shaavan Oct 18, 2024

TheBlueMatt commented Oct 16, 2024

shaavan commented Oct 18, 2024

shaavan commented Oct 18, 2024

TheBlueMatt commented Oct 22, 2024

shaavan commented Oct 22, 2024

shaavan commented Oct 22, 2024

TheBlueMatt commented Oct 22, 2024

Introduce RouteParametersConfig #3342

Are you sure you want to change the base?

Introduce RouteParametersConfig #3342

Conversation

shaavan commented Sep 26, 2024 • edited Loading

shaavan commented Sep 26, 2024

codecov bot commented Sep 26, 2024 • edited Loading

Codecov Report

vincenzopalazzo left a comment

Choose a reason for hiding this comment

vincenzopalazzo Oct 2, 2024

Choose a reason for hiding this comment

shaavan Oct 3, 2024

Choose a reason for hiding this comment

TheBlueMatt Oct 8, 2024

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt Oct 8, 2024

Choose a reason for hiding this comment

shaavan commented Oct 13, 2024

shaavan commented Oct 13, 2024

TheBlueMatt Oct 16, 2024

Choose a reason for hiding this comment

shaavan Oct 18, 2024

Choose a reason for hiding this comment

TheBlueMatt commented Oct 16, 2024

shaavan commented Oct 18, 2024

Update Role of RouteParamsOverride:

Cleanups:

shaavan commented Oct 18, 2024

TheBlueMatt commented Oct 22, 2024

shaavan commented Oct 22, 2024

shaavan commented Oct 22, 2024

TheBlueMatt commented Oct 22, 2024

shaavan commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading